Code structure #425

boocmp · 2025-02-04T12:14:58Z

The test code has been moved to the test/unit/ folder, replicating the src directory structure. In my opinion, this improves the efficiency of working with the codebase.

…rce_storage.rs

…ource_assembler.rs

…ing.rs

…_filter_cache.rs

github-actions

Rust Benchmark

Benchmark suite	Current: `d5a6a67`	Previous: `7919bdd`	Ratio
`rule-match-browserlike/brave-list`	`1766321560` ns/iter (`± 17538388`)	`1745226241` ns/iter (`± 10688991`)	`1.01`
`rule-match-first-request/brave-list`	`1018953` ns/iter (`± 7037`)	`1003256` ns/iter (`± 7610`)	`1.02`
`blocker_new/brave-list`	`209918857` ns/iter (`± 4443194`)	`210108247` ns/iter (`± 7007989`)	`1.00`
`memory-usage/brave-list-initial`	`41409969` ns/iter (`± 3`)	`41409969` ns/iter (`± 3`)	`1`
`memory-usage/brave-list-after-1000-requests`	`44005995` ns/iter (`± 3`)	`44005995` ns/iter (`± 3`)	`1`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions · 2025-02-04T19:16:51Z

src/filters/network.rs

@@ -894,13 +941,17 @@ impl NetworkFilter {
    /// emulate the behavior of hosts-style blocking.
    pub fn parse_hosts_style(hostname: &str, debug: bool) -> Result<Self, NetworkFilterError> {
        // Make sure the hostname doesn't contain any invalid characters
-        static INVALID_CHARS: Lazy<Regex> = Lazy::new(|| Regex::new("[/^*!?$&(){}\\[\\]+=~`\\s|@,'\"><:;]").unwrap());
+        static INVALID_CHARS: Lazy<Regex> =
+            Lazy::new(|| Regex::new("[/^*!?$&(){}\\[\\]+=~`\\s|@,'\"><:;]").unwrap());


_{reported by reviewdog 🐶}
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result

Cc @thypon @kdenhartog

github-actions · 2025-02-04T19:16:51Z

src/lists.rs

+            let original_rule = *filter
+                .raw_line
+                .clone()
+                .expect("All rules should be in debug mode");


_{reported by reviewdog 🐶}
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result

Cc @thypon @kdenhartog

github-actions · 2025-02-04T19:16:51Z

src/lists.rs

+            let original_rule = *filter
+                .raw_line
+                .clone()
+                .expect("All rules should be in debug mode");


_{reported by reviewdog 🐶}
[semgrep] expect or unwrap called in function returning a Result

Source: https://semgrep.dev/r/trailofbits.rs.panic-in-function-returning-result.panic-in-function-returning-result

Cc @thypon @kdenhartog

atuchin-m · 2025-02-04T19:37:53Z

tests/unit/optimizer.rs

+
+/*
+#[cfg(test)]
+mod optimization_tests_union_domain {


a duplicate? about the same block on line 190.

In this PR I just moved the code and fixed the compilation. The code moved as is, but I guess I can clean up it as well if everyone don't mind.

antonok-edm · 2025-02-05T15:32:22Z

@boocmp what exactly is the justification for moving all the tests? Codebase churn and personal opinions aside, it's considered idiomatic Rust to keep the unit tests close to the corresponding implementations.

Ref:

Unit Tests

The purpose of unit tests is to test each unit of code in isolation from the rest of the code to quickly pinpoint where code is and isn’t working as expected. You’ll put unit tests in the src directory in each file with the code that they’re testing. The convention is to create a module named tests in each file to contain the test functions and to annotate the module with cfg(test).

atuchin-m · 2025-02-05T21:09:50Z

src/url_parser/parser_regex.rs

@@ -1,77 +0,0 @@
-use regex::Regex;
-
-pub fn get_hostname_regex(url: &str) -> Option<(usize, (usize, usize))> {


is it a unused code?

boocmp · 2025-02-06T13:25:14Z

@boocmp what exactly is the justification for moving all the tests? Codebase churn and personal opinions aside, it's considered idiomatic Rust to keep the unit tests close to the corresponding implementations.

The first reason is to reduce the amount of code in files. It’s difficult to work with a file that has over a thousand lines of code, especially when most of it consists of tests. For example, a task like adding code to the end of a file becomes quite tedious you have to search for where the tests begin instead of simply pressing the End key. The second reason is search. I can configure a search for any substring while ignoring tests by simply excluding the test directory. The third reason is GitHub diffs. It’s much clearer to see where changes are made in the code versus in the tests, since GitHub only shows a few lines around the changes, it’s not always obvious otherwise. I’m not an expert in Rust, but this recommendation goes against everything I’ve seen in other languages and projects throughout my experience.

boocmp added 15 commits January 29, 2025 13:47

unit tests from network.rs moved to tests/unit/filters/network.rs

5d2f0d8

unit tests from cosmetic.rs moved to tests/unit/filters/cosmetic.rs

2892b25

Removed unused files from url_parser.

ae41283

unit tests from resource_storage.rs moved to tests/unit/filters/resou…

b61adfe

…rce_storage.rs

unit tests from resource_assembler.rs moved to tests/unit/filters/res…

0d10ae9

…ource_assembler.rs

unit tests from blocker.rs moved to tests/unit/blocker.rs

168ad77

unit tests from regex_manager.rs moved to tests/unit/regex_manager.rs

49e74da

unit tests from content_blocking.rs moved to tests/unit/content_block…

f586683

…ing.rs

unit tests from cosmetic_filter_cache.rs moved to tests/unit/cosmetic…

e236f2e

…_filter_cache.rs

unit tests from engine.rs moved to tests/unit/engine.rs

cbfd5d5

unit tests from lists.rs moved to tests/unit/lists.rs

0972777

unit tests from request.rs moved to tests/unit/request.rs

34ff68a

unit tests from optimizer.rs moved to tests/unit/optimizer.rs

140fba3

unit tests from utils.rs moved to tests/unit/utils.rs

9558e26

Rust formatter.

a6fd9a5

github-actions bot reviewed Feb 4, 2025

View reviewed changes

boocmp force-pushed the code_structure branch from 6c8853b to 8e315c2 Compare February 4, 2025 12:40

Rebase.

48df927

boocmp force-pushed the code_structure branch from 8e315c2 to 48df927 Compare February 4, 2025 12:50

boocmp self-assigned this Feb 4, 2025

boocmp marked this pull request as ready for review February 4, 2025 12:51

boocmp requested review from antonok-edm and atuchin-m February 4, 2025 12:51

cargo fmt in ci.yml

83a7e6e

github-actions bot reviewed Feb 4, 2025

View reviewed changes

github-actions bot added the needs-security-review label Feb 4, 2025

github-actions bot assigned kdenhartog and thypon Feb 4, 2025

Force LF checkout for win

d5a6a67

atuchin-m reviewed Feb 4, 2025

View reviewed changes

atuchin-m reviewed Feb 5, 2025

View reviewed changes

atuchin-m approved these changes Feb 5, 2025

View reviewed changes

boocmp merged commit 98aa69f into master Feb 11, 2025
11 checks passed

boocmp deleted the code_structure branch February 11, 2025 03:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code structure #425

Code structure #425

boocmp commented Feb 4, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot Feb 4, 2025

github-actions bot Feb 4, 2025

github-actions bot Feb 4, 2025

atuchin-m Feb 4, 2025

boocmp Feb 5, 2025

antonok-edm commented Feb 5, 2025

Unit Tests

atuchin-m Feb 5, 2025

boocmp Feb 6, 2025

boocmp commented Feb 6, 2025

		@@ -1,77 +0,0 @@
		use regex::Regex;

		pub fn get_hostname_regex(url: &str) -> Option<(usize, (usize, usize))> {

Code structure #425

Code structure #425

Conversation

boocmp commented Feb 4, 2025 • edited Loading

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Rust Benchmark

github-actions bot Feb 4, 2025

Choose a reason for hiding this comment

github-actions bot Feb 4, 2025

Choose a reason for hiding this comment

github-actions bot Feb 4, 2025

Choose a reason for hiding this comment

atuchin-m Feb 4, 2025

Choose a reason for hiding this comment

boocmp Feb 5, 2025

Choose a reason for hiding this comment

antonok-edm commented Feb 5, 2025

Unit Tests

atuchin-m Feb 5, 2025

Choose a reason for hiding this comment

boocmp Feb 6, 2025

Choose a reason for hiding this comment

boocmp commented Feb 6, 2025

boocmp commented Feb 4, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading